feat(screenshot): skip screenshot when preview URL returns non-2xx#289
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
=======================================
Coverage ? 86.60%
=======================================
Files ? 185
Lines ? 43494
Branches ? 4360
=======================================
Hits ? 37666
Misses ? 5828
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Closes aws-samples#287. Before: the processor only checked Page.navigate's errorText. An HTTP 4xx/5xx response or a 3xx redirect that doesn't resolve cleanly navigates 'successfully' from CDP's perspective, and a 404 / 503 / etc. PNG gets posted as if it were the deployed app. After: enable the Network CDP domain before navigating; tap ws.on('message') for Network.responseReceived events; record the latest type==='Document' response status for the navigated frame. After Page.loadEventFired + the 2s settle wait, throw if the captured status is non-2xx. The processor's existing catch logs and skips the comment post cleanly. Auth walls that return 200 (e.g. Vercel deployment protection) are out of scope — caller-side fix. Tests: 6 new in agentcore-browser.test.ts: - 200 → captures as before - 404 / 503 → throw with status in message - 301 redirect → throw (defensive) - non-Document responses (Stylesheet etc.) ignored - no Network events fire → falls through (pre-aws-samples#287 behaviour) Architecture-notes update + Starlight mirror sync. Test file is new on this branch; PR aws-samples#275 (the broader screenshot test PR) doesn't touch this file.
95d942d to
398a1c3
Compare
Closes aws-samples#97. Adds 53 jest tests across the four screenshot files that landed with PR aws-samples#241 + aws-samples#273 with no existing coverage: - github-webhook-verify.test.ts (14): SHA256 sign/verify, sm cache TTL + forceRefresh, ResourceNotFound, transparent re-fetch on signature mismatch / null fresh. - github-webhook.test.ts (15): missing body/sig, ping ack, non-deploy events ignored, malformed JSON, state/environment filters, SCREENSHOT_TARGET_ENVIRONMENT override, missing fields, dedup hit, happy path, rollback-on-invoke-failure, non-condition DDB error. - linear-issue-lookup.test.ts (18): regex covers extract / multi / bounds / case-sensitivity, prefix-routing happy path, case-insensitive prefix match, fallback for legacy rows + post-prefix-miss, null token skip, fuzzy-match guard, GraphQL errors / non-2xx / network failure. - github-webhook-processor.test.ts (15): empty / malformed body, missing fields, token resolve failure, PR retry exhaustion, OPEN-only filter, happy path with CloudFront-host URL assertion, screenshot/S3/comment failure modes (each non-fatal where appropriate), Linear branch fires / falls back to body / skips on no-id / no-resolve / non-fatal post. - agentcore-browser.test.ts (6): StartBrowserSession failures, full CDP exchange (Target.getTargets -> attach -> enable -> navigate -> loadEventFired -> captureScreenshot) returning PNG bytes, Stop invoked in finally even on CDP error, Stop's own failure logged not thrown, 403 unexpected-response surfaced, navigate errorText raised. All tests use jest mocks for AWS SDK clients + an in-test FakeWebSocket for the CDP stream so they run hermetically without real AWS or network. Existing 286/286 handler tests still pass.
…les#240 API Updates captureScreenshot budget-arg + high-entropy S3 key assertions to match main's aws-samples#240 versions, plus eslint formatting.
|
Rebased onto Net diff vs
#275's other two test files ( |
Closes #287.
Summary
Before: the processor only checked `Page.navigate`'s `errorText`. An HTTP 4xx/5xx response or a redirect that doesn't resolve cleanly navigates "successfully" from CDP's perspective, and a 404/503/etc. PNG gets posted as if it were the deployed app — exactly what krokoko's PR-241 review flagged ("a 404/login PNG gets posted as if it were the app").
After: enable the `Network` CDP domain before navigating; tap the raw `message` stream for `Network.responseReceived` events; record the latest `type==='Document'` response status for the navigated frame. After `Page.loadEventFired` + the 2s settle, throw if the captured status is non-2xx. The processor's existing `catch` swallows the error and skips the comment cleanly.
Auth walls that return 200 (e.g. Vercel deployment protection) are out of scope — caller-side fix; the screenshots guide already documents that.
Implementation notes
Test plan
Acceptance criteria (from #287)